feat(OpusEncoder): add opus_decode/encode_float binding#193
feat(OpusEncoder): add opus_decode/encode_float binding#193northwind33 wants to merge 4 commits intodiscordjs:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for floating-point Opus encoding and decoding operations by introducing opus_encode_float and opus_decode_float bindings to the Node.js wrapper.
- Adds
encodeFloatanddecodeFloatmethods to OpusEncoder class - Allocates a new
float*buffer for float operations - Updates documentation with usage examples for float32 operations
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/node-opus.h | Adds outFloat member variable and declares EncodeFloat and DecodeFloat methods |
| src/node-opus.cc | Implements float encoding/decoding methods, initializes outFloat buffer in constructor, and registers new methods |
| README.md | Documents the new float32 encoding/decoding API with usage examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (this->outPcm) delete this->outPcm; | ||
| if (this->outFloat) delete this->outFloat; |
There was a problem hiding this comment.
Using delete on an array allocated with new[] is undefined behavior and can cause memory leaks or crashes. Since outFloat is allocated with new float[channels * MAX_FRAME_SIZE] on line 66, it should be deallocated with delete[] instead of delete. The same issue exists with outPcm on line 76.
| if (this->outPcm) delete this->outPcm; | |
| if (this->outFloat) delete this->outFloat; | |
| if (this->outPcm) delete[] this->outPcm; | |
| if (this->outFloat) delete[] this->outFloat; |
| int frameSize = buf.Length() / 4 / this->channels; | ||
|
|
||
| int compressedLength = opus_encode_float(this->encoder, pcm, frameSize, &(this->outOpus[0]), MAX_PACKET_SIZE); | ||
|
|
There was a problem hiding this comment.
Missing error handling for the return value of opus_encode_float. According to the Opus API documentation, opus_encode_float can return negative values on error (same error codes as opus_decode_float). This function should check if compressedLength < 0 and throw an appropriate error, similar to how DecodeFloat checks for negative return values on line 243. Without this check, negative error codes could be passed to Buffer::Copy causing undefined behavior.
| if (compressedLength < 0) { | |
| Napi::TypeError::New(env, getDecodeError(compressedLength)).ThrowAsJavaScriptException(); | |
| return env.Null(); | |
| } |
northwind33
changed the title
|
I see there are conflicts due to the recent code updates. I'll rebase and fix them shortly. |
|
Sorry, but I just realized it's 3:00 AM here. I'll fix it later. |
northwind33
force-pushed
the
float_encode_decode
branch
from
2c12350 to
0d687fe
Compare
|
Regarding the remaining points raised by copilot:
I'm ready to push fixes for them. Before I begin, I have some questions; For the two points mentioned above, should I fix the existing bug in this PR while I'm at it? Or would you prefer I only fix my new code and open a separate issue/PR for the existing one? |
1 participant
Closes #171
Description
This PR adds bindings for
opus_decode_floatandopus_encode_floatto support floating-point operations.